util: add maxStringLength option to inspect function
#32392
Conversation
nodejs-github-bot
added
the
util
addaleax
added
the
semver-minor
There was a problem hiding this comment.
@rosaxny thank you very much for the PR! I am not certain in what way this is related to the referenced issue though, since that would still happen.
I also think we should not add this option in general without having a steong reason to do so. If someone wants to limit the steing size, that could be done after the stringification or by hooking in the unsupported stylize function. For those reasons I am -1 on this PR.
BridgeAR
added
semver-major
|
It is IMO also semver-major due to limiting information that was present before. |
|
@nodejs/TSC PTAL
@BridgeAR The process does not crash anymore when inspecting large strings in the REPL by default since it does not attempt to print a similarly large string. Ultimately, this is no different from the |
|
AFAIK it should already crash while checking for the string length. I am also no fan of the max array length. There are other possibilities to limit the total string length. The maxArrayLength is often to restrictive with the current default value. |
It does not.
I was skeptical about maxArrayLength at first too, but in the end I do feel very much that that was a good idea to introduce. It does keep the inspection output from becoming too overwhelming. Would you remove your objection if the default was set to |
|
Thanks for the PR! No objection, just a few thoughts:
This seems to be a side effect of the implementation, and not a likely use case. (I realize that this is likely copied from the
+1 for consistency with the rest of the API. At least I don't remember any APIs that use "str" instead of "string". There are lots of APIs that use "string", e.g.,
The meta string ( |
|
My comment has been addressed, thanks. |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/30192/ (:heavy_check_mark:) |
tniessen
changed the title
maxStrLength option to inspect functionmaxStringLength option to inspect function
addaleax
added
the
tsc-agenda
|
I’m putting this on the @nodejs/tsc agenda since there’s a standing objection from @BridgeAR who hasn’t engaged since that. @rosaxny Fyi, this would need to be rebased before it can land so that the merge conflict displayed by the GitHub UI is resolved. |
From the collaborator guide... I don't think we strictly need to involve the TSC for that. |
|
@cjihrig Hm yeah … I don’t want to just ignore somebody’s objection, even if I personally fully disagree with it. Unless there’s a reason to land this soon (Is there? I think the v14.0.0 semver-major cut has already passed? /cc @BethGriggs), I think waiting until the meeting should make sense? |
|
I'm pretty sure the semver-major cutoff has passed, but at least this can easily be semver-minor with the right default value 😄 |
rosaxxny
force-pushed
the
repl/edit-util-inspect-str-length
branch
from
63a4bb4
to
9f90e7b
Compare
|
@mmarchini @cjihrig I changed the default value to |
|
Thanks @rosaxny. The CI failures are unrelated. |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/30475/ (:heavy_check_mark:) |
|
Sorry for the late reply. I did not have the time to look at Node.js things in the last couple days. This does indeed fix #25478. I would still rather have a functionality that is more general purpose similar to the stylize function but this is probably more intuitive for most people who struggle with huge string length. I am therefore fine landing this and removed it from the TSC agenda. |
BridgeAR
removed
the
tsc-agenda
|
Landed in 944d862 🎉 |
Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #32392 PR-URL: #32744 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Notable changes: New file system APIs: * Added a new function, `fs.readv` (with sync and promisified versions). This function takes an array of `ArrayBufferView` elements and will write the data it reads sequentially to the buffers (Sk Sajidul Kadir). #32356 * A new overload is available for `fs.readSync`, which allows to optionally pass any of the `offset`, `length` and `position` parameters. #32460 Other changes: * dns: * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4 mapped IPv6 addresses (murgatroid99). #32183 * http: * The default maximum HTTP header size was changed from 8KB to 16KB (rosaxny). #32520 * n-api: * Calls to `napi_call_threadsafe_function` from the main thread can now return the `napi_would_deadlock` status in certain circumstances (Gabriel Schulhof). #32689 * util: * Added a new `maxStrLength` option to `util.inspect`, to control the maximum length of printed strings. Its default value is `Infinity` (rosaxny). #32392 * worker: * Added support for passing a `transferList` along with `workerData` to the `Worker` constructor (Juan José Arboleda). #32278 New core collaborators: With this release, we welcome three new Node.js core collaborators: * himself65. #32734 * flarna (Gerhard Stoebich). #32620 * mildsunrise (Alba Mendez). #32525 PR-URL: #32813
Notable changes: New file system APIs: * Added a new function, `fs.readv` (with sync and promisified versions). This function takes an array of `ArrayBufferView` elements and will write the data it reads sequentially to the buffers (Sk Sajidul Kadir). #32356 * A new overload is available for `fs.readSync`, which allows to optionally pass any of the `offset`, `length` and `position` parameters. #32460 Other changes: * dns: * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4 mapped IPv6 addresses (murgatroid99). #32183 * http: * The default maximum HTTP header size was changed from 8KB to 16KB (rosaxny). #32520 * n-api: * Calls to `napi_call_threadsafe_function` from the main thread can now return the `napi_would_deadlock` status in certain circumstances (Gabriel Schulhof). #32689 * util: * Added a new `maxStrLength` option to `util.inspect`, to control the maximum length of printed strings. Its default value is `Infinity` (rosaxny). #32392 * worker: * Added support for passing a `transferList` along with `workerData` to the `Worker` constructor (Juan José Arboleda). #32278 New core collaborators: With this release, we welcome three new Node.js core collaborators: * himself65. #32734 * flarna (Gerhard Stoebich). #32620 * mildsunrise (Alba Mendez). #32525 PR-URL: #32813
Notable changes: New file system APIs: * Added a new function, `fs.readv` (with sync and promisified versions). This function takes an array of `ArrayBufferView` elements and will write the data it reads sequentially to the buffers (Sk Sajidul Kadir). #32356 * A new overload is available for `fs.readSync`, which allows to optionally pass any of the `offset`, `length` and `position` parameters. #32460 Other changes: * dns: * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4 mapped IPv6 addresses (murgatroid99). #32183 * http: * The default maximum HTTP header size was changed from 8KB to 16KB (rosaxny). #32520 * n-api: * Calls to `napi_call_threadsafe_function` from the main thread can now return the `napi_would_deadlock` status in certain circumstances (Gabriel Schulhof). #32689 * util: * Added a new `maxStrLength` option to `util.inspect`, to control the maximum length of printed strings. Its default value is `Infinity` (rosaxny). #32392 * worker: * Added support for passing a `transferList` along with `workerData` to the `Worker` constructor (Juan José Arboleda). #32278 New core collaborators: With this release, we welcome three new Node.js core collaborators: * himself65. #32734 * flarna (Gerhard Stoebich). #32620 * mildsunrise (Alba Mendez). #32525 PR-URL: #32813
Refs: nodejs#25478 PR-URL: nodejs#32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #25478 PR-URL: #32392 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Added to `util.inspect` in nodejs/node#32392, in node v12.17, v13.13, v14+
Refs: #25478
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes